Search and Replace Widget #1755

jbicker · 2018-04-20T09:45:21Z

Search in workspace allows the user to search the whole workspace for textual occurences.
By default it searches for all text occurrences in all files in workspace case insensitive.

One can refine the search result by selecting "Match Case", "Match Whole Words" and "Use Regular Expressions" as well as using include and exclude fields with glob patterns.

The result tree is navigable by keyboard using cursor keys.

Additional control buttons above the search form will update the search results, collapse the result tree or reset the search form.

One can remove unwanted occurrences from the result tree by clicking the x on the right side of every tree element.

In addition to searching for text occurrences in the workspace the possibility to replace all matches are provided. The replace form field is hidden by default and can be expanded by clicking the small button on the left side of the search field.

In replace mode the selection of a result opens a diff editor widget where the left side shows the
original and the right side a preview with the replacements.

One is able to replace all or single occurrences in workspace.

akosyakov

I've noticed that the search does not find everything for me. Try to search for foo, it finds storage-service.spec.ts then adjust to fo, it does not find it anymore. Should be fixed separately?

akosyakov · 2018-04-20T14:26:04Z

packages/core/src/browser/icons/CollapseAll.svg

@@ -0,0 +1,6 @@
+<!--Copyright (c) Microsoft Corporation. All rights reserved.-->


@jbicker please check that files have our copyrights in addition

akosyakov · 2018-04-20T14:51:54Z

packages/search-in-workspace/src/browser/search-in-workspace-result-tree-widget.ts

+                const widget = currentTitle.owner;
+                const result = this.resultTree.get(widget.editor.uri.withoutScheme().toString());
+                if (result) {
+                    this.decorateEditor(result, widget);


Cases when result are undefined should be handled as well otherwise decoration won't be ever removed

akosyakov · 2018-04-20T14:56:28Z

packages/search-in-workspace/src/browser/search-in-workspace-result-tree-widget.ts

+        return new URI(uri).withScheme(MEMORY_TEXT).withQuery(lines.join("\n"));
+    }
+
+    protected decorateEditor(node: ResultNode, editorWidget: EditorWidget) {


The following code should handle removing decorations:

protected decorateEditor(node: ResultNode | undefined, editorWidget: EditorWidget) { const oldDecorations = this.appliedDecorations.get(key) || []; const appliedDecorations = editorWidget.editor.deltaDecorations({ newDecorations: this.createEditorDecorations(node), oldDecorations, uri: editorWidget.editor.uri.toString() }); this.appliedDecorations.set('search-in-workspace-matches', appliedDecorations); }

akosyakov · 2018-04-20T15:18:55Z

packages/editor/src/browser/editor.ts

+     * Replaces the text of source given in ReplacetextParams.
+     * @param params: ReplaceTextParams
+     */
+    replaceText(params: ReplaceTextParams): Promise<boolean>;


It would be better to have the general solution: #1756

Please open a follow-up issue to refactor it.

akosyakov · 2018-04-20T15:20:53Z

packages/core/src/browser/style/variables-bright.useable.css

@@ -142,6 +142,12 @@ is not optimized for dense, information rich UIs.
  --theia-removed-color0: rgba(230, 0, 0, 0.8);
  --theia-modified-color0: rgba(0, 100, 150, 0.8);

+  --theia-search-match-color0: rgba(234, 92, 0, 0.33);


Should we consider to move them to the search in workspace extension? They are not supposed to be used in core. If it is hard, please at least file a follow-up issue to make it easier to define themed variables for extensions.

I moved them to the extension and added new variables here. Not the best solution, a bit arbitrary maybe. I guess we should edit this with #1486.

Regarding themed variables for extensions: we shouldn't do such thing and leave every theme related variable in core.

akosyakov · 2018-04-20T15:22:07Z

packages/search-in-workspace/src/browser/search-in-workspace-frontend-module.ts

@@ -29,4 +49,16 @@ export default new ContainerModule(bind => {
        const client = ctx.container.get(SearchInWorkspaceClientImpl);
        return WebSocketConnectionProvider.createProxy(ctx.container, '/search-in-workspace', client);
    }).inSingletonScope();
+
+    bind(MemoryTextResourceResolver).toSelf().inSingletonScope();


How about InMemoryTextResource?

ok, sounds better.

akosyakov · 2018-04-20T15:23:16Z

packages/search-in-workspace/src/browser/search-in-workspace-result-tree-widget.ts

+import { MEMORY_TEXT } from "./memory-text-resource";
+import { FileResourceResolver } from "@theia/filesystem/lib/browser";
+
+export interface ResultNode extends ExpandableTreeNode, SelectableTreeNode {


What about putting supplementary types under SearchInWorkspaceResultTreeWidget. ResultNode, ResultLineNode sound generic. please review for other generic top level concepts

akosyakov · 2018-04-20T15:23:50Z

packages/search-in-workspace/src/browser/search-in-workspace-result-tree-widget.ts

+
+    protected changeEmitter: Emitter<Map<string, ResultNode>>;
+
+    constructor(


https://github.com/theia-ide/theia/wiki/Coding-Guidelines#property-injection please review for other occurences

akosyakov · 2018-04-20T15:28:07Z

packages/search-in-workspace/src/browser/styles/index.css

@@ -0,0 +1,315 @@
+.searchContainer {


please review for missing copyrights in css

akosyakov · 2018-04-20T15:35:23Z

packages/search-in-workspace/src/browser/styles/index.css

+    background: var(--theia-icon-clear);
+}
+
+.searchContainer .searchHeader .search-field {


please use more specific top level css class names to avoid collisions: theia-search-in-workspace-container

@kittaakos was suggesting to use t- prefix instead to make it shorter: t-siw-container

We can discuss css convention on next dev meeting, agree on something as a team and follow it.

svenefftinge · 2018-04-30T09:14:04Z

svenefftinge · 2018-05-10T19:58:23Z

The existing Search in Workspace command should open the view.

It still opens the old search command. It should be completely replaced with this (i.e. remove the existing functionality).

The alignment of the include/exclude input fields is not good:

svenefftinge

LGTM

kittaakos · 2018-05-15T17:10:28Z

@jbicker, here is an earlier issue that was dealing with vscode-ripgrep problems on Windows: [windows] Workspace search is broken. I hope it helps to identify the test failures on AppVeyor. Ping me if you need assistance.

akosyakov · 2018-05-16T07:26:59Z

packages/search-in-workspace/src/browser/search-in-workspace-result-tree-widget.ts

+            children: []
+        };
+
+        model.onSelectionChanged(nodes => {


it will leak, please collect all listeners

akosyakov · 2018-05-16T07:27:49Z

packages/search-in-workspace/src/browser/search-in-workspace-widget.ts

+            exclude: [],
+            maxResults: 500
+        };
+        this.resultTreeWidget.onChange(r => {


it will leak, please collect all listeners with this.toDispose.push

akosyakov · 2018-05-16T09:39:18Z

packages/search-in-workspace/src/browser/search-in-workspace-result-tree-widget.ts

            const node = nodes[0];
            if (SearchInWorkspaceResultLineNode.is(node)) {
                this.doOpen(node, true);
            }
-        });
+        }));

        model.onNodeRefreshed(() => this.changeEmitter.fire(this.resultTree));


this event listener also should be captured, please review all new widgets in this PR that they capture all event listeners

svenefftinge · 2018-05-16T19:21:40Z

seems like we are back to only flaky tests failing.
@jbicker could you squash the changes?

Signed-off-by: Jan Bicker <jan.bicker@typefox.io>

svenefftinge · 2018-05-17T08:00:55Z

Cool, please merge

simark · 2018-05-18T13:49:41Z

Hi Jan,

Awesome work! Could you add a test that searches for a string that contains a double quote? I think it now fails because we use shell: true.

You can also input fun things like

salut"; echo prout > /tmp/lol; "

which writes to /tmp/lol... So either we should not use shell: true, or we should sanitize the input.

jbicker · 2018-05-18T14:19:32Z

@simark, thanks for the hint. I will take care of it.
shell:true is actually needed, otherwise the glob patterns do not work.

kittaakos · 2018-05-18T14:20:20Z

Also, please clean up the branch if you do not need it, @jbicker. Thanks!

simark · 2018-05-18T14:30:56Z

@simark, thanks for the hint. I will take care of it.
shell:true is actually needed, otherwise the glob patterns do not work.

Can you explain why? From what I understand, the glob expressions are handled by rg, the shell should not play a role. More over, we probably don't want to depend on the particular shell that runs in the backend, different shells can have different behaviors.

jbicker · 2018-05-22T09:27:50Z

@simark: honestly I don't see anymore what the problem was without the shell option. So I removed it and also the quotes around the search term and the glob terms. So rg should handle the arguments, right?
The tests are running successfully and in theia it is working too.
Please check this PR: #1931

svenefftinge changed the title ~~[WIP] Search and Replace Widget implemented~~ [WIP] Search and Replace Widget Apr 20, 2018

jbicker force-pushed the SearchAndReplaceInWorkspace branch from 1b2a8d8 to bd9c5c1 Compare April 20, 2018 11:56

jbicker changed the title ~~[WIP] Search and Replace Widget~~ Search and Replace Widget implemented Apr 20, 2018

jbicker requested review from svenefftinge and akosyakov April 20, 2018 13:00

svenefftinge changed the title ~~Search and Replace Widget implemented~~ Search and Replace Widget Apr 20, 2018

akosyakov reviewed Apr 20, 2018

View reviewed changes

jbicker force-pushed the SearchAndReplaceInWorkspace branch 2 times, most recently from 43e34d2 to bf219ec Compare April 25, 2018 13:49

jbicker mentioned this pull request Apr 26, 2018

[search in workspace] For replace use more generalised solution #1788

Closed

jbicker force-pushed the SearchAndReplaceInWorkspace branch from 8024ad7 to 53b4380 Compare April 30, 2018 12:39

jbicker changed the title ~~Search and Replace Widget~~ [WIP] Search and Replace Widget May 7, 2018

jbicker force-pushed the SearchAndReplaceInWorkspace branch 7 times, most recently from 92fd1b9 to 42e4397 Compare May 9, 2018 11:39

jbicker changed the title ~~[WIP] Search and Replace Widget~~ Search and Replace Widget May 9, 2018

jbicker force-pushed the SearchAndReplaceInWorkspace branch from 42e4397 to 60fa482 Compare May 9, 2018 12:05

jbicker force-pushed the SearchAndReplaceInWorkspace branch 2 times, most recently from 93cb8f1 to 6eec2e2 Compare May 14, 2018 08:21

svenefftinge approved these changes May 14, 2018

View reviewed changes

jbicker force-pushed the SearchAndReplaceInWorkspace branch from 43936ac to b52950b Compare May 14, 2018 13:18

akosyakov reviewed May 16, 2018

View reviewed changes

Search and Replace Widget implemented

84b26f7

Signed-off-by: Jan Bicker <jan.bicker@typefox.io>

jbicker force-pushed the SearchAndReplaceInWorkspace branch from 8eb712e to 84b26f7 Compare May 17, 2018 06:42

jbicker merged commit 4996f9b into master May 17, 2018

svenefftinge deleted the SearchAndReplaceInWorkspace branch May 18, 2018 14:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Search and Replace Widget #1755

Search and Replace Widget #1755

jbicker commented Apr 20, 2018 •

edited

Loading

akosyakov left a comment

akosyakov Apr 20, 2018

jbicker Apr 25, 2018

akosyakov Apr 20, 2018 •

edited

Loading

akosyakov Apr 20, 2018 •

edited

Loading

akosyakov Apr 20, 2018

jbicker Apr 26, 2018

akosyakov Apr 20, 2018 •

edited

Loading

jbicker Apr 26, 2018

akosyakov Apr 20, 2018 •

edited

Loading

jbicker Apr 25, 2018

akosyakov Apr 20, 2018

jbicker Apr 25, 2018

akosyakov Apr 20, 2018

jbicker Apr 25, 2018

akosyakov Apr 20, 2018 •

edited

Loading

jbicker Apr 26, 2018

akosyakov Apr 20, 2018

jbicker Apr 26, 2018

svenefftinge commented Apr 30, 2018 •

edited by jbicker

Loading

svenefftinge commented May 10, 2018 •

edited

Loading

svenefftinge left a comment

kittaakos commented May 15, 2018

akosyakov May 16, 2018

akosyakov May 16, 2018

akosyakov May 16, 2018 •

edited

Loading

svenefftinge commented May 16, 2018

svenefftinge commented May 17, 2018

simark commented May 18, 2018

jbicker commented May 18, 2018

kittaakos commented May 18, 2018

simark commented May 18, 2018

jbicker commented May 22, 2018

		@@ -0,0 +1,6 @@
		<!--Copyright (c) Microsoft Corporation. All rights reserved.-->


		protected changeEmitter: Emitter<Map<string, ResultNode>>;

		constructor(

Search and Replace Widget #1755

Search and Replace Widget #1755

Conversation

jbicker commented Apr 20, 2018 • edited Loading

akosyakov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akosyakov Apr 20, 2018 • edited Loading

Choose a reason for hiding this comment

akosyakov Apr 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akosyakov Apr 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akosyakov Apr 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akosyakov Apr 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svenefftinge commented Apr 30, 2018 • edited by jbicker Loading

svenefftinge commented May 10, 2018 • edited Loading

svenefftinge left a comment

Choose a reason for hiding this comment

kittaakos commented May 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akosyakov May 16, 2018 • edited Loading

Choose a reason for hiding this comment

svenefftinge commented May 16, 2018

svenefftinge commented May 17, 2018

simark commented May 18, 2018

jbicker commented May 18, 2018

kittaakos commented May 18, 2018

simark commented May 18, 2018

jbicker commented May 22, 2018

jbicker commented Apr 20, 2018 •

edited

Loading

akosyakov Apr 20, 2018 •

edited

Loading

akosyakov Apr 20, 2018 •

edited

Loading

akosyakov Apr 20, 2018 •

edited

Loading

akosyakov Apr 20, 2018 •

edited

Loading

akosyakov Apr 20, 2018 •

edited

Loading

svenefftinge commented Apr 30, 2018 •

edited by jbicker

Loading

svenefftinge commented May 10, 2018 •

edited

Loading

akosyakov May 16, 2018 •

edited

Loading